Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: bulk ingester might skip listener requests #867

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

fabriziofortino
Copy link
Contributor

When BulkIngester uses the internal scheduler it might fail notifying the listener because the scheduler gets terminated before the submit is called. This happens because the close condition is signalled before submit is invoked. This PR fixes a regression introduced in #830

Copy link

cla-checker-service bot commented Aug 20, 2024

💚 CLA has been signed

@fabriziofortino fabriziofortino changed the title fix: bulk ingester might skip lister requests fix: bulk ingester might skip listener requests Aug 20, 2024
fabriziofortino added a commit to fabriziofortino/jackrabbit-oak that referenced this pull request Aug 20, 2024
@l-trotta l-trotta self-assigned this Aug 21, 2024
fabriziofortino added a commit to apache/jackrabbit-oak that referenced this pull request Aug 22, 2024
* OAK-11042: bump elastic 8.15.0 / lucene 9.11.1

* OAK-11029: add workaround for elastic/elasticsearch-java#867

* OAK-11029: better doc for workaround

* OAK-11029: improve bulkIngester#close

* OAK-11029: missing static modifier for ElasticBulkProcessorHandler#FAILED_DOC_COUNT_FOR_STATUS_NODE
@l-trotta
Copy link
Contributor

Hello @fabriziofortino, thank you for the contribution and for highlighting this issue. I have tested your solution and while it helps mitigate the problem, when dealing with many requests there's still a possibility that the latest listener tasks are ignored because of the shutdown. I'll do more testing to figure out a way to handle this reliably, I'll let you know so that we can fix this PR and merge it.

@l-trotta
Copy link
Contributor

@fabriziofortino do you mind if I push on this branch?

@fabriziofortino
Copy link
Contributor Author

@l-trotta no problem, go ahead!

@fabriziofortino
Copy link
Contributor Author

@l-trotta I have also noticed this: in the close() method when the scheduler is internal shutdownNow() gets called:

       if (scheduler != null && !isExternalScheduler) {
            scheduler.shutdownNow();
        }

This will try to cancel the already submitted tasks. Shouldn't we call shutdown() instead?

@l-trotta
Copy link
Contributor

@fabriziofortino you're correct, however with the new changes it shouldn't be a problem either way, you can take a look :)

Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for having caught this and the fix!

@l-trotta l-trotta merged commit 0c14348 into elastic:main Sep 12, 2024
4 checks passed
l-trotta added a commit that referenced this pull request Sep 12, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <laura.trotta@elastic.co>
Co-authored-by: Laura Trotta <153528055+l-trotta@users.noreply.github.com>
l-trotta added a commit that referenced this pull request Sep 12, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <laura.trotta@elastic.co>
Co-authored-by: Laura Trotta <153528055+l-trotta@users.noreply.github.com>
@fabriziofortino fabriziofortino deleted the bulk-ingester-fix branch September 12, 2024 16:14
l-trotta added a commit that referenced this pull request Sep 13, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <laura.trotta@elastic.co>
Co-authored-by: Laura Trotta <153528055+l-trotta@users.noreply.github.com>
blackwinter added a commit to hbz/limetrans that referenced this pull request Sep 30, 2024
Regression introduced in 8.15.0 appears to be fixed (elastic/elasticsearch-java#867).

This partially reverts commit a345a15.
@blackwinter
Copy link

This fix does not seem to be included in the 8.16.0 release. It looks like the 8.16 branch was cut on Sep 10, while this pull request was merged into 8.15 on Sep 12 and then never ported to 8.16. Can you please take a look? This affects our bulk indexing.

@l-trotta
Copy link
Contributor

@blackwinter you're correct, I backported it to 8.x where 8.16 was branched from, but forgot 8.16 itself. Sorry for the inconvenience, I'll port it right away, I'll let you know if we made it in time for it to be in 8.16.1

l-trotta added a commit that referenced this pull request Nov 20, 2024
* fix: bulk ingester might skip lister requests

* minor: fix style

* always waiting for listener to be done before closing

---------

Co-authored-by: Laura Trotta <laura.trotta@elastic.co>
Co-authored-by: Laura Trotta <153528055+l-trotta@users.noreply.github.com>
@l-trotta
Copy link
Contributor

@blackwinter no luck, didn't make it in time for the release. You can either wait for 8.16.2, which should be out next month, or you can use 8.16.1-SNAPSHOT from our snapshot repo, for example in gradle:

repositories {
    mavenCentral()
    maven {
        url = uri("https://snapshots.elastic.co/maven/")
    }
    ...
}
...
dependencies {
    ...
    implementation("co.elastic.clients:elasticsearch-java:8.16.1-SNAPSHOT")
    ...
}

@blackwinter
Copy link

Too bad. But thanks anyway. The snapshot seems to work (it's also available from Sonatype).

blackwinter added a commit to hbz/limetrans that referenced this pull request Dec 18, 2024
Skipping 8.16.0/.1 because they don't include the fix for elastic/elasticsearch-java#867.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants